crypto: replace rand<T>()%N idiom with unbiased rand_idx(N) #5392
Conversation
There was a problem hiding this comment.
There is also std::uniform_int_distribution. The light wallet server implemented a random_device then used uniform_int_distribution with it. Being a C++ functor make its slightly easier to compose in certain situations, although those use cases seem to be rare in this codebase.
src/crypto/crypto.h
Outdated
| /* Generate a random value between 0 and range_max-1 in an unbiased way | ||
| */ | ||
| template<typename T> | ||
| typename std::enable_if<std::is_pod<T>::value, T>::type rand_range(T range_max) { |
There was a problem hiding this comment.
is_pod is not sufficient, is_integral is a bit better.
src/crypto/crypto.h
Outdated
| { | ||
| res = rand<T>(); | ||
| } | ||
| while (res >= max_value); |
There was a problem hiding this comment.
Is the lower bound of the range implied 0 or implied std::numeric_limits<T>::min() ? This implementation is currently the former, but I'm not sure if allowing signed types is a good thing, it seems like the implementation could bust in those situations.
|
@vtnerd |
|
Sounds like I was unable to persuade you to use the STL function. Humbug. Might be worth handling the 0 case too. Or perhaps the documentation implies 0 should be filtered by caller? |
|
Now I'm persuaded to use the STL :) |
src/common/dns_utils.cpp
Outdated
| std::mt19937 gen(rd()); | ||
| std::uniform_int_distribution<int> dis(0, dns_urls.size() - 1); | ||
| size_t first_index = dis(gen); | ||
| size_t first_index = crypto::rand_range<size_t>(0, dns_urls.size() - 1); |
There was a problem hiding this comment.
Well, getting to update this at the same time is nice.
src/crypto/crypto.h
Outdated
| */ | ||
| template<typename T> | ||
| typename std::enable_if<std::is_integral<T>::value, T>::type rand_range(T range_min, T range_max) { | ||
| std::random_device rd; |
There was a problem hiding this comment.
We might want to define our own random generator to keep the behavior the same. The mt19937 isn't a crypto-secure generator:
struct random_device
{
typedef uint64_t result_type;
static constexpr result_type min() { return 0; }
static constexpr result_type max() { return result_type(-1); }
result_type operator()() const { return crypto::rand<result_type>(); }
};will work directly with uniform_int_distribution and doesn't do a syscall each time (similar to mt133937 usage).
|
Thank you so much for the suggestion, I think the patch is a lot better than original now :) |
|
I liked the original version with a single param. Maybe it can be added back as a simple forwarder to the (0, N-1) call ? |
|
@moneromooo-monero |
stoffu
changed the title
a2195b9 crypto: replace rand<T>()%N idiom with unbiased rand_idx(N) (stoffu)
No description provided.